-
Notifications
You must be signed in to change notification settings - Fork 13.4k
Allow enum
and union
literals to also create SSA values
#138759
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
No need to build `ArrayVec`s; just put everything exactly where it goes.
r? @Nadrieril rustbot has assigned @Nadrieril. Use |
Some changes occurred in compiler/rustc_codegen_ssa |
This comment has been minimized.
This comment has been minimized.
These commits modify the If this was unintentional then you should revert the changes before this PR is merged. |
r? codegen |
Spiderman meme |
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
Allow `enum` and `union` literals to also create SSA values Today, `Some(x)` always goes through an `alloca`, even in trivial cases where the niching means the constructor doesn't even change the value. For example, <https://rust.godbolt.org/z/6KG6PqoYz> ```rust pub fn demo(r: &i32) -> Option<&i32> { Some(r) } ``` currently emits the IR ```llvm define align 4 ptr `@demo(ptr` align 4 %r) unnamed_addr { start: %_0 = alloca [8 x i8], align 8 store ptr %r, ptr %_0, align 8 %0 = load ptr, ptr %_0, align 8 ret ptr %0 } ``` but with this PR it becomes just ```llvm define align 4 ptr `@demo(ptr` align 4 %r) unnamed_addr { start: ret ptr %r } ``` (Of course the optimizer can clean that up, but it'd be nice if it didn't have to -- especially in debug where it doesn't run. This is like rust-lang#123886, but that only handled non-simd `struct`s -- this PR generalizes it to all non-simd ADTs.) There's two commits you can review independently: 1. The first is simplifying how the aggregate handling works. Past-me wrote something overly complicated, needing arrayvecs and zipping, depending on a careful iteration order of the fields, and fragile enough that even for just structs it needed extra double-checks to make sure it even made the right variant. It's replaced with something far more direct that works just like `extract_field`: use the offset to put it in exactly the correct immediate in the `OperandValue`. This doesn't support anything new, just refactors -- including moving some things off `FunctionCx` that had no reason to be there. (I have no idea why my past self put them there.) 2. The second extends that work to support more ADTs. That means handing variants other than `FIRST_VARIANT`, handling the active field for unions, refactoring the discriminant code so the Place and Operand parts can share the calculation, etc.
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (875f416): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)Results (primary -2.3%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (secondary -2.1%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeResults (primary -0.1%, secondary -0.1%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Bootstrap: 775.297s -> 774.424s (-0.11%) |
This comment has been minimized.
This comment has been minimized.
It was already available as a generic parameter anyway, and it's not like we'll ever put a tag in the 5-billionth field.
9423566
to
a9031aa
Compare
Ben said I should re-roll this |
hello. |
ah, this is a "tomorrow" PR. poke me if I do not get to it then. |
@workingjubilee Friendly ping, as requested. |
// Real primitives only have one variant, but weird types like | ||
// `Result<!, !>` turn out to also be "Primitive", and dead code | ||
// like `Err(never)` needs to not ICE. | ||
assert_eq!(v, FIRST_VARIANT); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cogitating...
I wonder about
enum Never<T> {
Say(!),
Again<T>
}
The matter of what the "first" variant is probably gets funny when some are uninhabited...
pub(crate) fn supports_builder(layout: TyAndLayout<'tcx>) -> bool { | ||
match layout.backend_repr { | ||
BackendRepr::Memory { .. } if layout.is_zst() => true, | ||
BackendRepr::Scalar(_) | BackendRepr::ScalarPair(_, _) => true, | ||
_ => false, | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this builder used in so many places that this is simpler than just returning Result
? or Option
, I suppose
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some thoughts that occurred while slowly combing through
let layout = self.cx.spanned_layout_of(ty, span); | ||
!self.cx.is_backend_ref(layout) | ||
OperandRef::<Bx::Value>::supports_builder(layout) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not entirely sure why the logic was "isn't a ref" and is now "uses one of these representations"... was it relying on some upstream logical narrowing to e.g. not answer true
on BackendRepr::SimdVector
?
I suppose now that it's this it's more correct in any case, but still,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this probably isn't actually important, but unlike some of my other meandering comments I remain curious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a nitty review, it seems, my condolences.
if variant_index != untagged_variant { | ||
let niche_layout = layout.field(cx, tag_field.as_usize()); | ||
let niche_llty = cx.immediate_backend_type(niche_layout); | ||
let BackendRepr::Scalar(scalar) = niche_layout.backend_repr else { | ||
bug!("expected a scalar placeref for the niche"); | ||
}; | ||
// We are supposed to compute `niche_value.wrapping_add(niche_start)` wrapping | ||
// around the `niche`'s type. | ||
// The easiest way to do that is to do wrapping arithmetic on `u128` and then | ||
// masking off any extra bits that occur because we did the arithmetic with too many bits. | ||
let niche_value = variant_index.as_u32() - niche_variants.start().as_u32(); | ||
let niche_value = (niche_value as u128).wrapping_add(niche_start); | ||
let niche_value = niche_value & niche_layout.size.unsigned_int_max(); | ||
|
||
let niche_llval = cx.scalar_to_backend( | ||
Scalar::from_uint(niche_value, niche_layout.size), | ||
scalar, | ||
niche_llty, | ||
); | ||
Some((tag_field, niche_llval)) | ||
} else { | ||
None | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note to self: mostly just a move from line 242
Err(s) if s.is_uninit_valid() => { | ||
let bty = cx.type_from_scalar(s); | ||
cx.const_undef(bty) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm. for some reason this feels like it will bite my nose.
mir::Rvalue::Aggregate(ref kind, ref fields) => { | ||
let (variant_index, active_field_index) = match **kind { | ||
mir::AggregateKind::Adt(_, variant_index, _, _, active_field_index) => { | ||
(variant_index, active_field_index) | ||
} | ||
_ => (FIRST_VARIANT, None), | ||
}; | ||
|
||
let ty = rvalue.ty(self.mir, self.cx.tcx()); | ||
let ty = self.monomorphize(ty); | ||
let layout = self.cx.layout_of(ty); | ||
|
||
// `rvalue_creates_operand` has arranged that we only get here if | ||
// we can build the aggregate immediate from the field immediates. | ||
let mut inputs = ArrayVec::<Bx::Value, 2>::new(); | ||
let mut input_scalars = ArrayVec::<abi::Scalar, 2>::new(); | ||
for field_idx in layout.fields.index_by_increasing_offset() { | ||
let field_idx = FieldIdx::from_usize(field_idx); | ||
let op = self.codegen_operand(bx, &fields[field_idx]); | ||
let values = op.val.immediates_or_place().left_or_else(|p| { | ||
bug!("Field {field_idx:?} is {p:?} making {layout:?}"); | ||
}); | ||
let scalars = self.value_kind(op.layout).scalars().unwrap(); | ||
assert_eq!(values.len(), scalars.len()); | ||
inputs.extend(values); | ||
input_scalars.extend(scalars); | ||
let mut builder = OperandRef::builder(layout); | ||
for (field_idx, field) in fields.iter_enumerated() { | ||
let op = self.codegen_operand(bx, field); | ||
let fi = active_field_index.unwrap_or(field_idx); | ||
builder.insert_field(bx, variant_index, fi, op); | ||
} | ||
|
||
let output_scalars = self.value_kind(layout).scalars().unwrap(); | ||
itertools::izip!(&mut inputs, input_scalars, output_scalars).for_each( | ||
|(v, in_s, out_s)| { | ||
if in_s != out_s { | ||
// We have to be really careful about bool here, because | ||
// `(bool,)` stays i1 but `Cell<bool>` becomes i8. | ||
*v = bx.from_immediate(*v); | ||
*v = bx.to_immediate_scalar(*v, out_s); | ||
let tag_result = codegen_tagged_field_value(self.cx, variant_index, layout); | ||
match tag_result { | ||
Err(super::place::UninhabitedVariantError) => { | ||
// Like codegen_set_discr we use a sound abort, but could | ||
// potentially `unreachable` or just return the poison for | ||
// more optimizability, if that turns out to be helpful. | ||
bx.abort(); | ||
let val = OperandValue::poison(bx, layout); | ||
OperandRef { val, layout } | ||
} | ||
Ok(maybe_tag_value) => { | ||
if let Some((tag_field, tag_imm)) = maybe_tag_value { | ||
builder.insert_imm(tag_field, tag_imm); | ||
} | ||
}, | ||
); | ||
|
||
let val = OperandValue::from_immediates(inputs); | ||
assert!( | ||
val.is_expected_variant_for_type(self.cx, layout), | ||
"Made wrong variant {val:?} for type {layout:?}", | ||
); | ||
OperandRef { val, layout } | ||
builder.finalize(bx.cx()) | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should re-review this a last time before approving
} | ||
} | ||
|
||
pub fn finalize(&self, cx: &impl CodegenMethods<'tcx, Value = V>) -> OperandRef<'tcx, V> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...I'm sure this is a bit of a thick question to ask, but why is it called finalize
exactly? It feels like it should get a documentation comment?
/// general, as it might be a niche value or have a different size. | ||
/// | ||
/// It might also be an `Err` because the variant is uninhabited. | ||
pub(super) fn codegen_tagged_field_value<'tcx, V>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mmh, tagged_field_value
feels... off? I know it's an excruciatingly small detail, but codegen_tag_value
sounds more correct... namely, like it doesn't codegen anything in the univariant case, which is important?
@rustbot author |
On a more general note, it is always easier to review stuff when pure "move stuff around" things are in a separate commit from anything that actually changes logic. |
Today,
Some(x)
always goes through analloca
, even in trivial cases where the niching means the constructor doesn't even change the value.For example, https://rust.godbolt.org/z/6KG6PqoYz
currently emits the IR
but with this PR it becomes just
(Of course the optimizer can clean that up, but it'd be nice if it didn't have to -- especially in debug where it doesn't run. This is like #123886, but that only handled non-simd
struct
s -- this PR generalizes it to all non-simd ADTs.)The commits:
extract_field
: use the offset to put it in exactly the correct immediate in theOperandValue
. This doesn't support anything new, just refactors -- including moving some things offFunctionCx
that had no reason to be there. (I have no idea why my past self put them there.)FIRST_VARIANT
, handling the active field for unions, refactoring the discriminant code so the Place and Operand parts can share the calculation, etc.tag_field
beingusize
, so changed it toFieldIdx